Enforce top level keys in autoinstall user data #1899
Conversation
|
Additionally, should we consider this behavior change a breaking change? Since the interface hasn't changed, and I think the silent failures are more a bug than a feature (even though it was explicit), I think not. Opinions? |
Chris-Peterson444
force-pushed
the
enforce-top-level-keys-FR-5371
branch
3 times, most recently
from
7170f85
to
38ab469
Compare
Chris-Peterson444
force-pushed
the
enforce-top-level-keys-FR-5371
branch
2 times, most recently
from
d8e0504
to
0b8b236
Compare
| log.warning( | ||
| "Unrecognized keys may cause autoinstall to crash in future versions" | ||
| ) |
There was a problem hiding this comment.
Currently all this will do is print with WARNING to the log file. Do we see a need to use something like warning.warn to print to stderr instead? (Or stdout where the main server process is run)
There was a problem hiding this comment.
crash
I don't love this word, as the word crash will be seen to mean a defect.
stderr
No, that's not quite right. We need two cases to show the fault
- interactive - we need to show the results in a dialog that indicates that the autoinstall is bad. The existing crash dialog is a good clue, but we won't reuse it directly because this is a non-defect case.
- non-interactive - output needs to go to the same place as the rest of the UI, which may be on a serial console etc. Follow the existing context logging for that.
Also warn is the wrong mindset in that if we're supposed to be autoinstalling and we have stopped, then that is fatal to the program.
may cause
We have a distinct plan, it will cause, so let's set that text accordingly.
|
Force pushed w/ updates to change from a run time error to a warning for now. |
Subiquity currently ignores invalid top-level keys, but this has likely
been a major source of confusion of autoinstall capabilities.
In the future, the following autoinstall config will throw an
AutoinstallValidationError:
version: 1
interactive-sections:
- identity
literally-anything: lmao
This patch adds warnings to the logger and the machinery to
conditionally warn or error depending on the specified autoinstall
version.
Chris-Peterson444
force-pushed
the
enforce-top-level-keys-FR-5371
branch
from
0b8b236
to
7a5c552
Compare
| ): | ||
| self.message = f"Malformed autoinstall in {owner!r} section" | ||
| self.owner = owner | ||
| if not message: |
There was a problem hiding this comment.
A standard across Subiquity is to prefer "is not None" over this case. I suspect that's a little better here.
| message="top-level key 'version' is missing", | ||
| ) | ||
|
|
||
| if version <= 1: |
There was a problem hiding this comment.
if we're adding v1/v2 behavior in this PR, then I want to see the autoinstall-reference updated at the same time.
| log.warning( | ||
| "Unrecognized keys may cause autoinstall to crash in future versions" | ||
| ) |
There was a problem hiding this comment.
crash
I don't love this word, as the word crash will be seen to mean a defect.
stderr
No, that's not quite right. We need two cases to show the fault
- interactive - we need to show the results in a dialog that indicates that the autoinstall is bad. The existing crash dialog is a good clue, but we won't reuse it directly because this is a non-defect case.
- non-interactive - output needs to go to the same place as the rest of the UI, which may be on a serial console etc. Follow the existing context logging for that.
Also warn is the wrong mindset in that if we're supposed to be autoinstalling and we have stopped, then that is fatal to the program.
may cause
We have a distinct plan, it will cause, so let's set that text accordingly.
|
Closing, superseded by #1947 |
Subiquity has, until now, silently ignored invalid top-level keys.
This has likely been a major source of confusion of autoinstall
capabilities.
The following autoinstall config:
will now throw an AutoinstallValidationError.